Skip to content

Conversation

@shreyas02
Copy link

As shown in issue #171

@codecov
Copy link

codecov bot commented Apr 26, 2025

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (6694d26) to head (87fa43e).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/Visualization.jl 0.00% 24 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #174    +/-   ##
=======================================
  Coverage    0.00%   0.00%            
=======================================
  Files          11      11            
  Lines        3249    3349   +100     
=======================================
- Misses       3249    3349   +100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shreyas02
Copy link
Author

Hi @JordiManyer, I just noticed that this has not yet been merged. Are you looking/working on any other "fixes" for this problem? Thanks!

@JordiManyer
Copy link
Member

You need to pull from master.

In any case, I am not convinced this is the best way of doing things.

For instance: If we are now passing the triangulation as input, why are we also passing the parts? Can't we just add a new method that has the triangulation instead of the parts?

Also: within the createpvd functions, everything is serial and only gets used within the master processor. So for instance, in

Visualization.createpvd(parts::AbstractArray,args...;kwargs...)

both args and kwargs are passed directly to vtk and therefore are serial. When we save the pvd file, we only do it in the master processor. Everything is only done on the master processor.

Why do we need to know the parts? I think we do not.

Only two changes are necessary: First, we check for empty parts when we want to save the individual files, i.e we only save the ones which have something. Second, we need to allow some flexibility for the main processor in case processor 1 is empty.

struct DistributedPvd{T<:AbstractArray}
  pvds::T
  main::Int
end

DistributedPvd(pvds) = DistributedPvd(pvds,1)

function Base.setindex!(pvd::DistributedPvd,pvtk::AbstractArray,time::Real)
  map(pvtk) do pvtk
    !isnothing(pvtk) && vtk_save(pvtk)
  end
  map(pvtk,pvd.pvds) do pvtk,pvd
    if part == pvd.main
      pvd[time] = pvtk
    end
  end
end

and you can pass main as a constructor kwarg, or make the struct mutable and set it up the first time you access setindex!, for instance.
Something like that, but I think there is some more thinking to do.

Copilot AI review requested due to automatic review settings January 13, 2026 13:19
@shreyas02 shreyas02 marked this pull request as draft January 13, 2026 13:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for creating PVD (ParaView Data) files when triangulations are not present on all processes in a distributed setting. The main issue being addressed (issue #171) likely involved failures when some processes had empty triangulations.

Changes:

  • Added a parts field to DistributedPvd struct to track which parts have data
  • Introduced new createpvd methods that accept a DistributedTriangulation parameter and filter empty parts
  • Modified existing createpvd, savepvd, and setindex! methods to replace map_main calls with explicit map + if part == 1 checks
  • Updated NEWS.md to document the fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File Description
src/Visualization.jl Added parts field to DistributedPvd, new createpvd methods for DistributedTriangulation with empty part filtering, and updated existing methods to handle empty parts
NEWS.md Documented the fix for creating pvd files with empty triangulations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +228 to +236
function Visualization.createpvd(trian::DistributedTriangulation,parts::AbstractArray,args...;kwargs...)
nparts, new_parts = filter_empty_parts(parts,local_views(trian))
pvds = map(new_parts) do part
if part == 1
paraview_collection(args...;kwargs...)
end
end
DistributedPvd(pvds,new_parts)
end
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new createpvd method for handling DistributedTriangulation with potentially empty parts lacks test coverage. Consider adding a test case that creates a pvd file using a half-empty triangulation (similar to the half_empty_trian function in VisualizationTests.jl) to ensure the new functionality works correctly when triangulations are not present on all processes.

Copilot uses AI. Check for mistakes.

- Added support for Gridap v0.19, with distributed counterparts for the new feaures introduced. This includes support for polytopal meshes, polytopal methods and patch assembly. Since PR[#175](https://github.com/gridap/GridapDistributed.jl/pull/175).
- Added `MacroDiscreteModel`, which gives a global numbering and classification of the interfaces between processors. Since PR[#175](https://github.com/gridap/GridapDistributed.jl/pull/175).
- Added support to create pvd files when the triangulation is not there on all processes. To achive this, a new method for the createpvd function is added.
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling error: "achive" should be "achieve".

Suggested change
- Added support to create pvd files when the triangulation is not there on all processes. To achive this, a new method for the createpvd function is added.
- Added support to create pvd files when the triangulation is not there on all processes. To achieve this, a new method for the createpvd function is added.

Copilot uses AI. Check for mistakes.
- Added support to create pvd files when the triangulation is not there on all processes. To achive this, a new method for the createpvd function is added.

## [0.4.7] - 2025-03-04
## [0.4.7] 2025-03-04
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent date format. All other entries use the format "## [version] - YYYY-MM-DD" with a hyphen before the date, but this entry is missing the hyphen.

Suggested change
## [0.4.7] 2025-03-04
## [0.4.7] - 2025-03-04

Copilot uses AI. Check for mistakes.
## [0.4.8] - 2025-06-11

### Added
### Fixed
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace after "Fixed" should be removed.

Suggested change
### Fixed
### Fixed

Copilot uses AI. Check for mistakes.
map(pvtk) do pvtk
if !isnothing(pvtk)
vtk_save(pvtk)
end
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace after the closing brace should be removed.

Suggested change
end
end

Copilot uses AI. Check for mistakes.
map_main(pvd.pvds) do pvd
vtk_save(pvd)
map(pvd.pvds, pvd.parts) do pvd, part
if part == 1
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace after the part == 1 condition should be removed.

Suggested change
if part == 1
if part == 1

Copilot uses AI. Check for mistakes.
end
end
map(pvtk,pvd.pvds,pvd.parts) do pvtk,pvd,part
if part == 1
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using the non-filtered createpvd method (lines 247-254) with potentially empty triangulations, part == 1 could correspond to a process with an empty triangulation, making pvtk equal to nothing. Assigning nothing to the paraview collection may cause an error. Consider adding an additional check: if part == 1 && !isnothing(pvtk) to ensure the pvtk is valid before adding it to the collection.

Suggested change
if part == 1
if part == 1 && !isnothing(pvtk)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants